Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start annotating project-system for nullability. #4404

Closed
wants to merge 20 commits into from

Conversation

Pilchie
Copy link
Member

@Pilchie Pilchie commented Dec 17, 2018

e7bb224 annotates everything except the generated code for Microsoft.VisualStudio.ProjectSystem.Managed.dll

@Pilchie Pilchie requested a review from a team as a code owner December 17, 2018 17:47
@jmarolf
Copy link
Contributor

jmarolf commented Dec 17, 2018

    public static T FirstOrDefault<T, TArg>(this IEnumerable<T> source, Func<T, TArg, bool> predicate, TArg arg)

Why aren't we saying this returns T?

{
if (_attributeValueProviderMap.TryGetValue(propertyName, out SourceAssemblyAttributePropertyValueProvider provider) &&
!await IsAssemblyInfoPropertyGeneratedByBuild(propertyName))
{
#pragma warning disable CS8604
await provider.SetPropertyValueAsync(unevaluatedPropertyValue);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review - not sure if we should be allowing nulls here, and if not, what we should do? Throw/skip the call/etc?

@@ -384,7 +387,7 @@ private IEnumerable<string> CollectInputs(BuildUpToDateCheckLogger logger)
if (_customInputs.Count != 0)
{
logger.Verbose("Adding " + UpToDateCheckInput.SchemaName + " inputs:");
foreach (string input in _customInputs.Select(_configuredProject.UnconfiguredProject.MakeRooted))
foreach (string input in _customInputs.Select(s => _configuredProject.UnconfiguredProject.MakeRooted(s)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like another compiler bug, but I wasn't sure how to file it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually gives better performance anyway as the compiler caches the delegate for lambdas but not for method groups.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that changed very recently, but I could be wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pilchie
Copy link
Member Author

Pilchie commented Dec 17, 2018

@jcouv @cston @jaredpar would love if you could skim through this.

@@ -147,15 +147,19 @@ private void ProcessProjectBuildFailure(IProjectRuleSnapshot snapshot)
//
// We still forward those 'removes' of references, sources, etc onto Roslyn to avoid duplicate/incorrect results when the next
// successful build occurs, because it will be diff between it and this failed build.
_context.LastDesignTimeBuildSucceeded = snapshot.IsEvaluationSucceeded();
if (_context != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume all this is rewritten in @davkean 's language service branch. But it feels like we would want to throw something in this case

@jmarolf
Copy link
Contributor

jmarolf commented Dec 17, 2018

Remaining warnings are all from auto-generated files in the obj directory. Is there no way to turn this analysis off for generated code? Will we need to put #nullable enable at the top of all of our source files and remove the project item?

@Pilchie
Copy link
Member Author

Pilchie commented Dec 17, 2018

See dotnet/roslyn#31875

@@ -390,8 +390,11 @@ private VersionCompatibilityData GetVersionCompatibilityData()

if (versionCompatData != null)
{
// This is never null after InitializeAsync (which is called during package load)
Version ourVSVersion = _ourVSVersion!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we want to throw an object disposed exception in this case?

@cston
Copy link
Member

cston commented Dec 18, 2018

    private readonly TaskCompletionSource<object> _projectLoadedInHost = new TaskCompletionSource<object>();

TaskCompletionSource<object?> here and below?


Refers to: src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/UnconfiguredProjectTasksService.cs:21 in 1c5b339. [](commit_id = 1c5b339, deletion_comment = False)

@davkean
Copy link
Member

davkean commented May 3, 2019

@Pilchie How do you want to move this forward?

@Pilchie
Copy link
Member Author

Pilchie commented May 3, 2019

I think this is the wrong approach. Instead I think we should opt-in to C# 8, but leave nullable off, and let people do PRs a file/directory at a time using #nullable until it reaches critical mass.

@Pilchie Pilchie closed this May 3, 2019
@davkean
Copy link
Member

davkean commented May 3, 2019

👍 Yeah I was thinking as much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants